Skip to content

Ensure that api:openapi:export produces JSON with unescaped forward slashes#3368

Merged
dunglas merged 1 commit into
api-platform:2.5from
Ocramius:fix/dump-openapi-schema-without-escaping-slashes
Jan 31, 2020
Merged

Ensure that api:openapi:export produces JSON with unescaped forward slashes#3368
dunglas merged 1 commit into
api-platform:2.5from
Ocramius:fix/dump-openapi-schema-without-escaping-slashes

Conversation

@Ocramius

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tickets
License MIT
Doc PR

This ensures that the generated OpenAPI schema JSON dump does not contain forward-slashes, such as "#\/foo\/bar\/MyModel", which confuses tooling relying on direct string match in referenced models (most notably, the IntelliJ OpenAPI/Swagger tooling)

use Symfony\Component\Console\Output\BufferedOutput;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

class SwaggerCommandUnitTest extends KernelTestCase

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: a unit test was much simpler and efficient to write - the current integration test is not something I could work with anyway, since I don't run MongoDB on any system I maintain (including my dev environment) by choice

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there was a an attempt to remove the hard dev dependency to MongoDB (#2703), but it has been added again by #2843 (an issue with PHPStan it seems).

By default though, the Behat tests are run without MongoDB.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that can be achieved again at some point: for now, I'm happy with adding a unit test, and providing more unit tests in future, shall I encounter isolated issues, like this one.

@teohhanhui teohhanhui Feb 17, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP extensions are no longer required to be loaded - thanks to Roave/BetterReflection and PhpStorm stubs!

https://github.com/phpstan/phpstan/releases/tag/0.12.4

We should be able to do this once we upgrade to phpstan ^0.12.4! 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we still can't remove the MongoDB packages, because we'd get errors such as:

 ------ -------------------------------------------------------------------------------------------- 
  Line   src/Bridge/Doctrine/Common/Util/IdentifierManagerTrait.php (in context of anonymous class)  
 ------ -------------------------------------------------------------------------------------------- 
  47     Class Doctrine\ODM\MongoDB\DocumentManager not found.                                       
  86     Call to static method hasType() on an unknown class Doctrine\ODM\MongoDB\Types\Type.        
  87     Call to static method getType() on an unknown class Doctrine\ODM\MongoDB\Types\Type.        
 ------ --------------------------------------------------------------------------------------------

😞

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ondrejmirtes Do you perhaps have any suggestions on how this could be resolved? I see there are MongoDB ODM stubs in https://github.com/phpstan/phpstan-doctrine/tree/master/stubs

Is there a way to tell phpstan that these types "exist" purely based on the stubs?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #3431 for an attempt.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to tell phpstan that these types "exist" purely based on the stubs?

You'd have to make the class available for the autoloader. See: https://phpstan.org/user-guide/autoloading

Comment thread tests/Bridge/Symfony/Bundle/Command/SwaggerCommandUnitTest.php Outdated
@Ocramius
Ocramius force-pushed the fix/dump-openapi-schema-without-escaping-slashes branch from 0dd8feb to 0d31896 Compare January 29, 2020 10:45
@alanpoulain

Copy link
Copy Markdown
Member

Couldn't this be a BC if the client has been relying on the escaped slashes?

@Ocramius

Copy link
Copy Markdown
Contributor Author

Doubt that, but there is indeed a change in dumped definitions. Forward slashes do not impose an escaping sequence risk.

@dunglas
dunglas merged commit 66cc242 into api-platform:2.5 Jan 31, 2020
@dunglas

dunglas commented Jan 31, 2020

Copy link
Copy Markdown
Member

Thanks @Ocramius!

@Ocramius
Ocramius deleted the fix/dump-openapi-schema-without-escaping-slashes branch February 12, 2020 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants